Add static_file.name and .basename Liquid attributes
#5264
Conversation
| @@ -97,6 +97,7 @@ def write(dest) | |||
|
|
|||
| def to_liquid | |||
| { | |||
| "filename" => File.basename(name, extname), | |||
There was a problem hiding this comment.
Or simply use name in order to simple call file.name in liquid?
| @@ -1,6 +1,6 @@ | |||
| module Jekyll | |||
| class StaticFile | |||
| attr_reader :relative_path, :extname | |||
| attr_reader :relative_path, :extname, :name | |||
There was a problem hiding this comment.
This is a short way of defining:
def name
@name
endTherefore, the best thing to do here is to set @name in the initialize method using the code you have on line 100 and then, when you are constructing to_liquid below, you can just call the name method instead of re-calculating its value. Does that make sense?
There was a problem hiding this comment.
@parkr yes, but then it breaks the tests as name used to be the whole name of the file including the extension. 馃槙
|
Well done, @DirtyF! This is great. I left a comment above which should help you get this ready for a merge. 馃槃 And well done adding a test! I would love to also see a test for the 馃帀 |
|
I'm not against this but the LGTM is being witheld since this is a breaking change and there is no backwards compatibility being kept until 4.0, can somebody explain to me what the ramifications of this change are? |
|
@envygeeks I didn't intend a breaking change, only wanted to add an extra Liquid shortcut to access the name of a static file without having to use liquid filters to remove the extra parts. |
|
@DirtyF Well, the question really is, can I still access it the old way if I want to? |
|
/cc @jekyll/stability |
| @@ -97,6 +97,7 @@ def write(dest) | |||
|
|
|||
| def to_liquid | |||
| { | |||
| "name" => File.basename(name, extname), | |||
There was a problem hiding this comment.
I'm fine with adding this to Liquid. When you do static_file.name in Ruby, it expects something else, though?
There was a problem hiding this comment.
From what I understand, in the initialize function, name is the complete name of the file including the extension.
I just modified the output in Liquid, to be able to get only the name without the extension, already available as extname. I did not want to modify the current behavior, as it would break all current tests.
As a total beginner, I have no clue how to properly achieve this. I just thought that we got to keep name as it is, and just add filename in the to_liquid function to be able to use both.
|
@envygeeks what do you mean 'the old way'? all i see in this change is additions, nothing breaking |
|
Interesting, this change LGTM. I think the main issue was the possibility that @DirtyF, would you please leave a comment next to your addition to |
|
@parkr I don't get what you mean 馃槙
So it doesn't match I completed the documentation with explicit examples for liquid metadata available for static files. |
|
@DirtyF I'd suggest naming it Other, unrelated note: would we see an improvement using Jekyll Drops for these things? |
|
@spudowiar you're right Documentation updated to reflect the changes.
|
|
LGTM. /cc @jekyll/core |
|
@parkr yes it is. |
parkr
changed the title
static_file.name and .basename Liquid attributes
|
This should eventually become a Drop. For now, LGTM. @jekyllbot: merge +minor |
Disclaimer: These are my first ruby lines 馃槄
A proposal to fix #5263